-
Notifications
You must be signed in to change notification settings - Fork 3.9k
api: remove nullable from StatusOr value methods #12338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
api: remove nullable from StatusOr value methods #12338
Conversation
I'm a little bit worried that this is getting into specifics of one Nullable annotation vs another; I know they aren't all equivalent and there were limitations lifted with later annotations. Given that everything is |
I was trying to use this type in our code, with the result of unexpected warnings from IDE There are plenty of such warnings in this repo as well, e.g. grpc-java/xds/src/main/java/io/grpc/xds/ClusterResolverLoadBalancer.java Lines 118 to 119 in 1a7042a
|
All arguments/return types in gRPC should be Nullable from a tool's perspective. We don't have NonNull enabled at the package level. To my current understanding, if these changes here are causing changes in the IDE, then the IDE is broken. |
From a tool's perspective - default nullability is unknown. Why |
The suggested approach doesn't actually work with javax.annotation.Nullable:
But the |
Correct, should be newer annotations, e.g. jspecify. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning is a bit different, but it is best to remove this at present.
It someone wants it nullable, that should be defined via the type parameter, e.g. `StatusOr<@nullable Foo> foo`
It someone wants it nullable, that should be defined via the type parameter, e.g. `StatusOr<@nullable Foo> foo`
It someone wants it nullable, that should be defined via the type parameter, e.g.
StatusOr<@Nullable Foo> foo